Skip to content

[6.1.7 Cherry-pick] Fix | Scope configurable retry logic assembly resolution to opt-in callers - #4664

Merged
paulmedynski merged 3 commits into
release/6.1from
dev/automation/pr-4547-to-6.1.7
Sep 9, 2026
Merged

[6.1.7 Cherry-pick] Fix | Scope configurable retry logic assembly resolution to opt-in callers#4664
paulmedynski merged 3 commits into
release/6.1from
dev/automation/pr-4547-to-6.1.7

Conversation

@github-actions

@github-actions github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Cherry-pick of #4547 (7c5daeb) into release/6.1 failed due to merge conflicts.

Please resolve manually:

git fetch origin
git checkout dev/automation/pr-4547-to-6.1.7
git cherry-pick 7c5daeb228a44114fe7d9608c71826c7bf95da7c
# resolve conflicts
git push origin dev/automation/pr-4547-to-6.1.7 --force

To resolve, run:  git cherry-pick 7c5daeb
@github-actions github-actions Bot added this to the 6.1.7 milestone Sep 8, 2026
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Sep 8, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

…llers (#4547)

* Scope configurable retry logic assembly resolution

SqlConfigurableRetryLogicLoader subscribed a handler to
AssemblyLoadContext.Default.Resolving in its constructor and never removed
it. Because SqlConfigurableRetryLogicManager builds that loader on the
default RetryLogicProvider path, simply reading
SqlCommand.RetryLogicProvider or SqlConnection.RetryLogicProvider installed
a permanent, process-wide assembly resolution hook.

The hook then participated in resolving every assembly the host application
failed to find, even though the application had not configured any custom
retry logic type. It also probed Environment.CurrentDirectory, which is
ambient process state unrelated to where the application's binaries live,
so assemblies could be resolved from an unintended location.

Applications observed this as load failures, and in #2214 as a stack
overflow, originating inside SqlClient for assemblies unrelated to
SqlClient.

Changes:

- Probe AppContext.BaseDirectory instead of Environment.CurrentDirectory.
- Subscribe the resolving handler only for the duration of the Type.GetType
  call in LoadType, and remove it in a finally block.
- Skip type resolution entirely when no retryLogicType is configured.
  retryLogicType is optional while retryMethod is required, so
  configurations selecting a built-in retry method previously still ran the
  custom type resolution path.

Together these mean the handler is never installed unless the application
explicitly configured a custom retry logic type, and is gone again as soon
as that type has been resolved.

Only .NET is affected; the .NET Framework code path does not use
AssemblyLoadContext.

Refs #2214, #2134

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

* Test | Address review feedback on retry logic assembly probing tests

Replaces the internals-based assertions in the configurable retry logic
regression tests with a behavioural probe, and restores the UTF-8 BOM that
was dropped from the functional test file.

The unit test previously read AssemblyLoadContext's private _resolving
field to check whether a handler was still subscribed. That reflects into
runtime internals we do not own, so the value cannot simply be exposed
internally as review suggested. The functional test took a different but
also problematic approach, mutating Environment.CurrentDirectory, which is
process-wide state and unsafe under parallel test execution.

Both now plant a file that is not a valid assembly in the loader's probing
directory (AppContext.BaseDirectory) under a name no other component could
request, then assert that Assembly.Load reports it as not found. A
subscribed handler would locate that file and surface
BadImageFormatException instead, so the assertion discriminates cleanly
while observing only public behaviour and touching no shared process state.

Verified by temporarily reintroducing the unconditional subscription: all
four unit tests and the functional test fail, and pass again once removed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

* Test | Cover the successful retry logic type resolution path

The existing tests only covered the code paths where the assembly probing
handler is never subscribed. The path that legitimately subscribes it, a
configured custom retry logic type that actually resolves, was untested, so
nothing verified that the handler is removed again afterwards.

Add a test that resolves a retry logic factory out of the loader's probing
directory and asserts that no probing handler remains subscribed once the
loader has been constructed. An invocation counter on the factory confirms the
configured type really was resolved and used, rather than the loader silently
falling back to the built-in factory.

Verified the test is sensitive to both behaviours it covers: pointing the
loader's probing directory elsewhere makes it fail, and restoring the
unconditional handler subscription makes it fail.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

* Test | Widen probe file cleanup to non-IO failures

File.Delete can fail with UnauthorizedAccessException as well as IOException.
Catching only the latter meant a cleanup failure could surface as a test
failure that had nothing to do with the behaviour under test.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

* Keep retry logic assembly probing active during provider construction

Addresses review feedback that scoping the assembly resolving handler to type
resolution alone could break existing consumers whose configured retryLogicType
has private dependencies.

The handler is now subscribed before LoadType and removed only after
CreateInstance has run the configured type's constructor and invoked its retry
method, so dependency loads triggered during construction are still resolved.

Adds Switch.Microsoft.Data.SqlClient.UseLegacyRetryLogicAssemblyResolution as an
escape hatch that restores the process-lifetime handler. The switch restores
lifetime only; the probing directory remains AppContext.BaseDirectory, so it
cannot re-enable the binary planting vector.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

* Treat a whitespace-only retryLogicType as not configured

A whitespace value has no type to resolve, so it previously installed the
resolving handler, attempted resolution and then fell back to the built-in
factory. Skipping the subscription reaches the same provider without changing
assembly resolution behavior on the application's behalf.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

* Remove the UseLegacyRetryLogicAssemblyResolution app context switch

The switch restored the process-lifetime assembly resolving handler for the
one case that scoping cannot cover: a custom retry logic provider whose
private dependency is first touched after the provider has been constructed.

Shipping a supported way to permanently reinstate a process-wide handler on
AssemblyLoadContext.Default works against the point of the change. The driver
should not be altering assembly resolution for the whole application on
behalf of configurable retry logic, and an affected provider has a simple fix
of its own: reference the dependency normally so it lands in deps.json, or
register a resolving handler in the application.

The handler is now always subscribed only while a configured provider is
being resolved and constructed, and only when a custom retry logic type has
been configured.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

* Refactor retry assembly resolution subscription

Encapsulate the temporary AssemblyLoadContext resolving handler in an
IDisposable subscription so cleanup is tied to a using scope. Remove the
handler immediately when custom type resolution falls back to the built-in
factory, and add direct unit coverage for disposal.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801

---------

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e8018b20-0e43-4de9-93dd-c8080a81a801
@benrr101
benrr101 marked this pull request as ready for review September 8, 2026 20:41
@benrr101
benrr101 requested a review from a team as a code owner September 8, 2026 20:41
Copilot AI balanced review requested due to automatic review settings September 8, 2026 20:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new unit-test file has two branch-specific compilation blockers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Scopes configurable retry assembly resolution to custom providers and probes from the application base directory.

Changes:

  • Adds temporary, disposable assembly-resolution subscriptions.
  • Skips custom resolution when no custom type is configured.
  • Adds regression coverage for handler lifecycle.
File summaries
File Description
SqlConfigurableRetryLogicLoader.cs Implements scoped assembly resolution.
SqlConfigurableRetryLogicLoaderTest.cs Adds loader unit tests.
SqlConfigurableRetryLogicTest.cs Adds functional regression coverage.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@benrr101 benrr101 changed the title [6.1.7 Cherry-pick - CONFLICTS] Fix | Scope configurable retry logic assembly resolution to opt-in callers [6.1.7 Cherry-pick] Fix | Scope configurable retry logic assembly resolution to opt-in callers Sep 8, 2026

@priyankatiwari08 priyankatiwari08 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against #4547. The 6.1 backport includes the complete production fix and regression coverage, with appropriate branch-specific adjustments; no code concerns.

@paulmedynski
paulmedynski enabled auto-merge (squash) September 9, 2026 15:12
@paulmedynski

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).
3 pipeline(s) were filtered out due to trigger conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants